qcontainer: clean up QEMU machine option related code #1922
Conversation
This change adds a machine_type parameter to DevContainer class init method and modifies execute_qemu() to add machine option to QEMU command line automatically. Signed-off-by: Huan Xiong <[email protected]>
| @@ -1493,7 +1493,8 @@ def sort_key(dev): | |||
| cmd += "numactl -m %s " % n | |||
|
|
|||
| # Start constructing devices representation | |||
| devices = qcontainer.DevContainer(qemu_binary, self.name, | |||
| machine_type = params.get('machine_type').split(':', 1)[-1] | |||
| devices = qcontainer.DevContainer(qemu_binary, self.name, machine_type, | |||
There was a problem hiding this comment.
Well I like the idea of using the machine-type used in testing when probing qemu, but it would affect negative testing with bad machine types. Have you encountered a situation where -M virt was not enough?
| "stdio -vnc none" % qemu_binary, | ||
| timeout=10, ignore_status=True, | ||
| shell=True, verbose=False)) | ||
| c = "echo -e 'help\nquit' | %s -M %s -monitor stdio -vnc none" % \ |
There was a problem hiding this comment.
c (nor c1, ...) match the pylint style (minimum 3 chars).
| '{ "execute": "quit" }\'') | ||
| c2 = '(sleep 1; cat )' | ||
| c3 = '%s -M %s -qmp stdio -vnc none | grep return | grep RAND91' % \ | ||
| (self.__qemu_binary, self.__machine_type) |
There was a problem hiding this comment.
Well, I slightly prefer having it embedded directly (for the sake of memory consumption), but it can be a bit easier to read like that. Anyway the variables would have to be named properly to express what they stand for and more importantly, do not mix style fixes with refactors, nor new features. They have to be in separate commits. (a good indication of a bad commit is when you need to use also or and in the commit message to join 2 different things)
| @@ -93,34 +96,18 @@ def get_qmp_cmds(qemu_binary, workaround_qemu_qmp_crash=False): | |||
| return cmds | |||
|
|
|||
| self.__state = -1 # -1 synchronized, 0 synchronized after hotplug | |||
| self.__machine_type = machine_type | |||
There was a problem hiding this comment.
I'd use the term __base_machine_type or __early_machine_type as later in the code the machine_type could be different/changed/....
| @@ -42,19 +42,22 @@ class DevContainer(object): | |||
| """ | |||
| # General methods | |||
|
|
|||
| def __init__(self, qemu_binary, vmname, strict_mode="no", | |||
| def __init__(self, qemu_binary, vmname, machine_type, strict_mode="no", | |||
There was a problem hiding this comment.
Always add new arguments to the end of the list to avoid breaking existing calls... What I mean is someone might be using DevContainer(bin, name, "yes") directly (and there is at least one test io-github-autotest-qemu/qemu/tests/slof_device_tree.py) that would be affected by your change. When you introduce the new argument as last providing a safe default, the change will be backward compatible. The question is what is backward compatible as s390 does not define virt and arm won't work without -M...
There was a problem hiding this comment.
Actually we could perhaps use machine_type=none. Not sure whether we use any features that would be affected.
There was a problem hiding this comment.
This PR is a bit difficult to assess. There is the slof_device_tree test that will be affected by this which means there could be other tests as well. Maybe we could use none and get backward-compatible version.
Bigger problem is the negative testing. We still need to query for things and with bad machine_type that won't work. Detecting the bad machine_type would effectively lead to the original implementation with basic_qemu_command based on the return of qemu execution so we would gain nothing (maybe a better default, but does -M affect the return of -device help? Or other queries?)
Anyway I might be wrong so feel free to disagree, but you have to convince me about the usefulness vs. what this change can break.
As for the style fixes, please extract them and feel free to send them separately or as part of this (or instead of this)
This change adds a machine_type parameter to DevContainer class init
method and modifies execute_qemu() to add machine option to QEMU command
line automatically.
Signed-off-by: Huan Xiong [email protected]